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

Address some new Roslyn nullability warnings #35782

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ internal static string FormatFileLoadExceptionMessage(string? fileName, int hRes
else
GetMessageForHR(hResult, new StringHandleOnStack(ref message));

return string.Format(format, fileName, message);
return string.Format(format!, fileName, message);
}

[DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ internal static RuntimeAssembly GetExecutingAssembly(ref StackCrawlMark stackMar
{
RuntimeAssembly? retAssembly = null;
GetExecutingAssemblyNative(new StackCrawlMarkHandle(ref stackMark), ObjectHandleOnStack.Create(ref retAssembly));
return retAssembly;
return retAssembly!;
}

// Get the assembly that the current code is running from.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ internal AssemblyBuilder(AssemblyName name,
new StackCrawlMarkHandle(ref stackMark),
(int)access,
ObjectHandleOnStack.Create(ref retAssembly));
_internalAssemblyBuilder = (InternalAssemblyBuilder)retAssembly;
_internalAssemblyBuilder = (InternalAssemblyBuilder)retAssembly!;

_assemblyData = new AssemblyBuilderData(_internalAssemblyBuilder, access);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ internal static RuntimeAssembly InternalLoad(AssemblyName assemblyName,
throwOnFileNotFound,
ObjectHandleOnStack.Create(ref assemblyLoadContext),
ObjectHandleOnStack.Create(ref retAssembly));
return retAssembly;
return retAssembly!;
}

[DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ internal unsafe ref struct StringHandleOnStack
{
private void* _ptr;

internal StringHandleOnStack([NotNull] ref string? s)
internal StringHandleOnStack(ref string? s)
{
_ptr = Unsafe.AsPointer(ref s);
}
Expand All @@ -31,7 +31,7 @@ private ObjectHandleOnStack(void* pObject)
_ptr = pObject;
}

internal static ObjectHandleOnStack Create<T>([NotNull] ref T o) where T : class?
internal static ObjectHandleOnStack Create<T>(ref T o) where T : class?
{
return new ObjectHandleOnStack(Unsafe.AsPointer(ref o));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,7 @@ private KeyValuePair<TKey, TValue>[] CreateKeyValueArray(int count, IEnumerator<

public TValue Lookup(TKey key)
{
bool found = TryGetValue(key, out TValue value);

if (!found)
if (!TryGetValue(key, out TValue value))
{
Debug.Assert(key != null);
Exception e = new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ private DictionaryToMapAdapter()
internal V Lookup<K, V>(K key) where K : notnull
{
IDictionary<K, V> _this = Unsafe.As<IDictionary<K, V>>(this);
bool keyFound = _this.TryGetValue(key, out V value);

if (!keyFound)
if (!_this.TryGetValue(key, out V value))
{
Debug.Assert(key != null);
Exception e = new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ private IReadOnlyDictionaryToIMapViewAdapter()
internal V Lookup<K, V>(K key) where K : notnull
{
IReadOnlyDictionary<K, V> _this = Unsafe.As<IReadOnlyDictionary<K, V>>(this);
bool keyFound = _this.TryGetValue(key, out V value);

if (!keyFound)
if (!_this.TryGetValue(key, out V value))
{
Debug.Assert(key != null);
Exception e = new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString()));
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Array.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ public static void Resize<T>([NotNull] ref T[]? array, int newSize)
Copy(larray, 0, newArray, 0, larray.Length > newSize ? newSize : larray.Length);
array = newArray;
}

Debug.Assert(array != null);
}

public static Array CreateInstance(Type elementType, params long[] lengths)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ bool IProducerConsumerCollection<T>.TryAdd(T item)
/// <remarks>For <see cref="ConcurrentQueue{T}"/>, this operation will attempt to remove the object
/// from the beginning of the <see cref="ConcurrentQueue{T}"/>.
/// </remarks>
bool IProducerConsumerCollection<T>.TryTake(out T item) => TryDequeue(out item);
bool IProducerConsumerCollection<T>.TryTake([MaybeNullWhen(false)] out T item) => TryDequeue(out item);

/// <summary>
/// Gets a value that indicates whether the <see cref="ConcurrentQueue{T}"/> is empty.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public void AwaitOnCompleted<TAwaiter, TStateMachine>(
AwaitOnCompleted(ref awaiter, ref stateMachine, ref m_task);

internal static void AwaitOnCompleted<TAwaiter, TStateMachine>(
ref TAwaiter awaiter, ref TStateMachine stateMachine, [NotNull] ref Task<TResult>? taskField)
ref TAwaiter awaiter, ref TStateMachine stateMachine, ref Task<TResult>? taskField)
where TAwaiter : INotifyCompletion
where TStateMachine : IAsyncStateMachine
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public void AwaitOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter, ref
}

internal static void AwaitOnCompleted<TAwaiter, TStateMachine>(
ref TAwaiter awaiter, ref TStateMachine stateMachine, [NotNull] ref StateMachineBox? box)
ref TAwaiter awaiter, ref TStateMachine stateMachine, ref StateMachineBox? box)
where TAwaiter : INotifyCompletion
where TStateMachine : IAsyncStateMachine
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public static class LazyInitializer
/// </para>
/// </remarks>
public static T EnsureInitialized<T>([NotNull] ref T? target) where T : class =>
Volatile.Read(ref target) ?? EnsureInitializedCore(ref target);
Volatile.Read(ref target!) ?? EnsureInitializedCore(ref target);

/// <summary>
/// Initializes a target reference type with the type's default constructor (slow path)
Expand Down Expand Up @@ -101,7 +101,7 @@ private static T EnsureInitializedCore<T>([NotNull] ref T? target) where T : cla
/// </para>
/// </remarks>
public static T EnsureInitialized<T>([NotNull] ref T? target, Func<T> valueFactory) where T : class =>
Volatile.Read(ref target) ?? EnsureInitializedCore(ref target, valueFactory);
Volatile.Read(ref target!) ?? EnsureInitializedCore(ref target, valueFactory);

/// <summary>
/// Initialize the target using the given delegate (slow path).
Expand Down Expand Up @@ -133,9 +133,10 @@ private static T EnsureInitializedCore<T>([NotNull] ref T? target, Func<T> value
/// <param name="initialized">A reference to a boolean that determines whether the target has already
/// been initialized.</param>
/// <param name="syncLock">A reference to an object used as the mutually exclusive lock for initializing
/// <paramref name="target"/>. If <paramref name="syncLock"/> is null, a new object will be instantiated.</param>
/// <paramref name="target"/>. If <paramref name="syncLock"/> is null, and if the target hasn't already
/// been initialized, a new object will be instantiated.</param>
/// <returns>The initialized value of type <typeparamref name="T"/>.</returns>
public static T EnsureInitialized<T>([AllowNull] ref T target, ref bool initialized, [NotNull] ref object? syncLock)
public static T EnsureInitialized<T>([AllowNull] ref T target, ref bool initialized, [NotNullIfNotNull("syncLock")] ref object? syncLock)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This [NotNullIfNotNull("syncLock")] on the syncLock parameter is basically saying "the method won't set this ref parameter to null". I wonder if we have other places where we should be adding such an attribute... to my knowledge these if the first time we've used NotNullIfNotNull to refer to the same parameter it's attributed on.

Copy link
Member

@stephentoub stephentoub May 4, 2020

Choose a reason for hiding this comment

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

For example, everywhere we have a ref T foo parameter, should we be annotating it as [NotNullIfNotNull("foo")] (just in case T was nullable)? That'd be kind of awful. @jcouv, what's the recommended approach here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Along those lines, Volatile.Read<T>(ref T value) could be a candidate for an [In]-like attribute. Basically: "The caller must explicitly use the ref keyword to pass an address to this routine (no implicit in conversion allowed), but the routine promises not to assign a new value to the provided address." That attribute would also imply [NotNullIfNotNull("sameParam")] by construction.

{
// Fast path.
if (Volatile.Read(ref initialized))
Expand Down Expand Up @@ -190,11 +191,12 @@ private static T EnsureInitializedCore<T>([AllowNull] ref T target, ref bool ini
/// <param name="initialized">A reference to a boolean that determines whether the target has already
/// been initialized.</param>
/// <param name="syncLock">A reference to an object used as the mutually exclusive lock for initializing
/// <paramref name="target"/>. If <paramref name="syncLock"/> is null, a new object will be instantiated.</param>
/// <paramref name="target"/>. If <paramref name="syncLock"/> is null, and if the target hasn't already
/// been initialized, a new object will be instantiated.</param>
/// <param name="valueFactory">The <see cref="System.Func{T}"/> invoked to initialize the
/// reference or value.</param>
/// <returns>The initialized value of type <typeparamref name="T"/>.</returns>
public static T EnsureInitialized<T>([AllowNull] ref T target, ref bool initialized, [NotNull] ref object? syncLock, Func<T> valueFactory)
public static T EnsureInitialized<T>([AllowNull] ref T target, ref bool initialized, [NotNullIfNotNull("syncLock")] ref object? syncLock, Func<T> valueFactory)
{
// Fast path.
if (Volatile.Read(ref initialized))
Expand Down Expand Up @@ -239,11 +241,12 @@ private static T EnsureInitializedCore<T>([AllowNull] ref T target, ref bool ini
/// <typeparam name="T">The type of the reference to be initialized. Has to be reference type.</typeparam>
/// <param name="target">A reference of type <typeparamref name="T"/> to initialize if it has not already been initialized.</param>
/// <param name="syncLock">A reference to an object used as the mutually exclusive lock for initializing
/// <paramref name="target"/>. If <paramref name="syncLock"/> is null, a new object will be instantiated.</param>
/// <paramref name="target"/>. If <paramref name="syncLock"/> is null, and if the target hasn't already
/// been initialized, a new object will be instantiated.</param>
/// <param name="valueFactory">The <see cref="System.Func{T}"/> invoked to initialize the reference.</param>
/// <returns>The initialized value of type <typeparamref name="T"/>.</returns>
public static T EnsureInitialized<T>([NotNull] ref T? target, [NotNull] ref object? syncLock, Func<T> valueFactory) where T : class =>
Volatile.Read(ref target) ?? EnsureInitializedCore(ref target, ref syncLock, valueFactory);
public static T EnsureInitialized<T>([NotNull] ref T? target, [NotNullIfNotNull("syncLock")] ref object? syncLock, Func<T> valueFactory) where T : class =>
Volatile.Read(ref target!) ?? EnsureInitializedCore(ref target, ref syncLock, valueFactory);

/// <summary>
/// Ensure the target is initialized and return the value (slow path). This overload works only for reference type targets.
Expand Down Expand Up @@ -272,7 +275,8 @@ private static T EnsureInitializedCore<T>([NotNull] ref T? target, [NotNull] ref
}
}

return target!; // TODO-NULLABLE: Compiler can't infer target's non-nullness (https://github.com/dotnet/roslyn/issues/37300)
Debug.Assert(target != null);
return target;
}

/// <summary>
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Threading/ref/System.Threading.cs
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,10 @@ public static void MemoryBarrierProcessWide() { }
public static partial class LazyInitializer
{
public static T EnsureInitialized<T>([System.Diagnostics.CodeAnalysis.NotNullAttribute] ref T? target) where T : class { throw null; }
public static T EnsureInitialized<T>([System.Diagnostics.CodeAnalysis.AllowNullAttribute] ref T target, ref bool initialized, [System.Diagnostics.CodeAnalysis.NotNullAttribute] ref object? syncLock) { throw null; }
public static T EnsureInitialized<T>([System.Diagnostics.CodeAnalysis.AllowNullAttribute] ref T target, ref bool initialized, [System.Diagnostics.CodeAnalysis.NotNullAttribute] ref object? syncLock, System.Func<T> valueFactory) { throw null; }
public static T EnsureInitialized<T>([System.Diagnostics.CodeAnalysis.AllowNullAttribute] ref T target, ref bool initialized, [System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("syncLock")] ref object? syncLock) { throw null; }
public static T EnsureInitialized<T>([System.Diagnostics.CodeAnalysis.AllowNullAttribute] ref T target, ref bool initialized, [System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("syncLock")] ref object? syncLock, System.Func<T> valueFactory) { throw null; }
public static T EnsureInitialized<T>([System.Diagnostics.CodeAnalysis.NotNullAttribute] ref T? target, System.Func<T> valueFactory) where T : class { throw null; }
public static T EnsureInitialized<T>([System.Diagnostics.CodeAnalysis.NotNullAttribute] ref T? target, [System.Diagnostics.CodeAnalysis.NotNullAttribute] ref object? syncLock, System.Func<T> valueFactory) where T : class { throw null; }
public static T EnsureInitialized<T>([System.Diagnostics.CodeAnalysis.NotNullAttribute] ref T? target, [System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("syncLock")] ref object? syncLock, System.Func<T> valueFactory) where T : class { throw null; }
}
public partial struct LockCookie
{
Expand Down