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

Help compiler enforce nullability annotations #32090

Merged
merged 12 commits into from
Feb 14, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ public static partial class Environment

// Terminates this process with the given exit code.
[DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)]
[DoesNotReturn]
private static extern void _Exit(int exitCode);

[DoesNotReturn]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

namespace System.Collections.Generic
Expand All @@ -15,7 +16,7 @@ private ReferenceEqualityComparer()
{
}

public bool Equals(T x, T y)
public bool Equals([AllowNull] T x, [AllowNull] T y)
Copy link
Member

@safern safern Feb 11, 2020

Choose a reason for hiding this comment

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

Since T is constraint to class, can we instead annotate T as T? ? #Resolved

Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

Indeed. I'd missed that. Thanks #Resolved

Copy link
Member Author

@jcouv jcouv Feb 12, 2020

Choose a reason for hiding this comment

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

Hum, it looks like this file is compiled into more than one project. One with nullability enabled and one without. Reverted to use the attribute. #Resolved

Copy link
Member

@stephentoub stephentoub Feb 12, 2020

Choose a reason for hiding this comment

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

Can you use the annotation and just add #nullable enable at the top of the file as well? That's what we've done in other cases like this, where we've not yet gotten to every project that includes it. #Resolved

{
return ReferenceEquals(x, y);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -918,11 +918,13 @@ public TValue this[TKey key]
// as these are uncommonly needed and when inlined are observed to prevent the inlining
// of important methods like TryGetValue and ContainsKey.

[DoesNotReturn]
private static void ThrowKeyNotFoundException(object key)
{
throw new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString()));
}

[DoesNotReturn]
private static void ThrowKeyNullException()
{
throw new ArgumentNullException("key");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;

namespace System.Collections.Immutable
{
Expand Down Expand Up @@ -120,6 +121,7 @@ internal interface IImmutableListQueries<T> : IReadOnlyList<T>
/// The first element that matches the conditions defined by the specified predicate,
/// if found; otherwise, the default value for type <typeparamref name="T"/>.
/// </returns>
[return: MaybeNull]
T Find(Predicate<T> match);

/// <summary>
Expand Down Expand Up @@ -193,6 +195,7 @@ internal interface IImmutableListQueries<T> : IReadOnlyList<T>
/// The last element that matches the conditions defined by the specified predicate,
/// if found; otherwise, the default value for type <typeparamref name="T"/>.
/// </returns>
[return: MaybeNull]
T FindLast(Predicate<T> match);

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public ImmutableHashSet<T> Remove(T item)
[Pure]
public bool TryGetValue(T equalValue, out T actualValue)
{
int hashCode = _equalityComparer.GetHashCode(equalValue);
int hashCode = _equalityComparer.GetHashCode(equalValue!);
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 should we remove the [DisallowNull] on that GetHashCode, or add [DisallowNull] here?
(same question below) #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

In general IEqualityComparer<T>.GetHashCode (e.g. StringComparer.GetHashCode throws ArgumentNullException for null) even if some implementations do, so we can't remove [DisallowNull]. We could consider adding [DisallowNull] to this T equalValue, but then we'd be saying that null wasn't allowed even if the developer provided a comparer implementation that did actually allow null. This is an example where we lack sufficient expressivity. I guess for now it'd be best to add [DisallowNull], and then see what kind of issues that causes; we can remove [DisallowNull] later on, but it'll be harder to add it later on. #Resolved

HashBucket bucket;
if (_root.TryGetValue(hashCode, out bucket))
{
Expand Down Expand Up @@ -639,7 +639,7 @@ private static bool IsSupersetOf(IEnumerable<T> other, MutationInput origin)
private static MutationResult Add(T item, MutationInput origin)
{
OperationResult result;
int hashCode = origin.EqualityComparer.GetHashCode(item);
int hashCode = origin.EqualityComparer.GetHashCode(item!);
Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

But now it seems like that would actually not just be for TryGetValue, but really all operations on the hash set, at which point we'd be better off using a notnull constraint? Maybe instead of changing the annotations, open an issue about it and keep the ! changes you have here. #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

(And actually, the right answer is probably to do a null check prior to calling GetHashCode, as we do in HashSet<T>.) #Resolved

HashBucket bucket = origin.Root.GetValueOrDefault(hashCode);
var newBucket = bucket.Add(item, origin.EqualityComparer, out result);
if (result == OperationResult.NoChangeRequired)
Expand All @@ -658,7 +658,7 @@ private static MutationResult Add(T item, MutationInput origin)
private static MutationResult Remove(T item, MutationInput origin)
{
var result = OperationResult.NoChangeRequired;
int hashCode = origin.EqualityComparer.GetHashCode(item);
int hashCode = origin.EqualityComparer.GetHashCode(item!);
HashBucket bucket;
var newRoot = origin.Root;
if (origin.Root.TryGetValue(hashCode, out bucket))
Expand All @@ -680,7 +680,7 @@ private static MutationResult Remove(T item, MutationInput origin)
/// </summary>
private static bool Contains(T item, MutationInput origin)
{
int hashCode = origin.EqualityComparer.GetHashCode(item);
int hashCode = origin.EqualityComparer.GetHashCode(item!);
HashBucket bucket;
if (origin.Root.TryGetValue(hashCode, out bucket))
{
Expand All @@ -701,7 +701,7 @@ private static MutationResult Union(IEnumerable<T> other, MutationInput origin)
var newRoot = origin.Root;
foreach (var item in other.GetEnumerableDisposable<T, Enumerator>())
{
int hashCode = origin.EqualityComparer.GetHashCode(item);
int hashCode = origin.EqualityComparer.GetHashCode(item!);
HashBucket bucket = newRoot.GetValueOrDefault(hashCode);
OperationResult result;
var newBucket = bucket.Add(item, origin.EqualityComparer, out result);
Expand Down Expand Up @@ -812,7 +812,7 @@ private static MutationResult Except(IEnumerable<T> other, IEqualityComparer<T>
var newRoot = root;
foreach (var item in other.GetEnumerableDisposable<T, Enumerator>())
{
int hashCode = equalityComparer.GetHashCode(item);
int hashCode = equalityComparer.GetHashCode(item!);
HashBucket bucket;
if (newRoot.TryGetValue(hashCode, out bucket))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public int GetHashCode(HashSet<T>? obj)
{
foreach (T t in obj)
{
hashCode = hashCode ^ (_comparer.GetHashCode(t) & 0x7FFFFFFF);
hashCode = hashCode ^ (_comparer.GetHashCode(t!) & 0x7FFFFFFF); // TODO2
}
} // else returns hashcode of 0 for null hashsets
return hashCode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ internal override int TotalCount()
// This passes functionality down to the underlying tree, clipping edges if necessary
// There's nothing gained by having a nested subset. May as well draw it from the base
// Cannot increase the bounds of the subset, can only decrease it
public override SortedSet<T> GetViewBetween(T lowerValue, T upperValue)
public override SortedSet<T> GetViewBetween([AllowNull] T lowerValue, [AllowNull] T upperValue)
{
if (_lBoundActive && Comparer.Compare(_min, lowerValue) > 0)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ public static IEqualityComparer<SortedSet<T>> CreateSetComparer(IEqualityCompare
/// <param name="set2">The second set.</param>
/// <param name="comparer">The fallback comparer to use if the sets do not have equal comparers.</param>
/// <returns><c>true</c> if the sets have equal contents; otherwise, <c>false</c>.</returns>
internal static bool SortedSetEquals(SortedSet<T> set1, SortedSet<T> set2, IComparer<T> comparer)
internal static bool SortedSetEquals(SortedSet<T>? set1, SortedSet<T>? set2, IComparer<T> comparer)
{
if (set1 == null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ private SortedSetEqualityComparer(IComparer<T>? comparer, IEqualityComparer<T>?
}

// Use _comparer to keep equals properties intact; don't want to choose one of the comparers.
public bool Equals(SortedSet<T> x, SortedSet<T> y) => SortedSet<T>.SortedSetEquals(x, y, _comparer);
public bool Equals(SortedSet<T>? x, SortedSet<T>? y) => SortedSet<T>.SortedSetEquals(x, y, _comparer);

// IMPORTANT: this part uses the fact that GetHashCode() is consistent with the notion of equality in the set.
public int GetHashCode(SortedSet<T> obj)
Expand All @@ -37,7 +37,7 @@ public int GetHashCode(SortedSet<T> obj)
{
foreach (T t in obj)
{
hashCode = hashCode ^ (_memberEqualityComparer.GetHashCode(t) & 0x7FFFFFFF);
hashCode = hashCode ^ (_memberEqualityComparer.GetHashCode(t!) & 0x7FFFFFFF); // TODO2
}
}
// Returns 0 for null sets.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,12 @@ internal virtual bool TryGetExports(ImportDefinition definition, [NotNullWhen(tr
if (multipleExports != null)
{
multipleMatches = multipleExports;
// TODO2 singleMatch dould be null when returning true
singleMatch = null!;
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 the [NotNullWhen(...)] annotation on method seems incorrect #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

Yes, please remove it. Thanks. #Resolved

}
else
{
singleMatch = singleExport;
singleMatch = singleExport!;
}
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,15 @@ private static bool IsLazyGenericType(Type genericType)

private static bool TryGetCastFunction(Type genericType, bool isOpenGeneric, Type[] arguments, [NotNullWhen(true)] out Func<Export, object>? castFunction)
{
castFunction = null;
castFunction = null!; // TODO2
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 the [NotNullWhen(...)] annotation on method seems incorrect #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

Yes, please remove it. The dangers of the compiler not having checked those annotations ;) #Resolved


if (genericType == LazyOfTType)
{
if (!isOpenGeneric)
{
castFunction = ExportServices.CreateStronglyTypedLazyFactory(arguments[0].UnderlyingSystemType, null);
}
// castFunction
return true;
}

Expand All @@ -189,6 +190,7 @@ private static bool TryGetCastFunction(Type genericType, bool isOpenGeneric, Typ
{
castFunction = ExportServices.CreateStronglyTypedLazyFactory(arguments[0].UnderlyingSystemType, arguments[1].UnderlyingSystemType);
}
// castFunction
return true;
}

Expand All @@ -200,6 +202,7 @@ private static bool TryGetCastFunction(Type genericType, bool isOpenGeneric, Typ
{
castFunction = new ExportFactoryCreator(genericType).CreateStronglyTypedExportFactoryFactory(arguments[0].UnderlyingSystemType, null);
}
// castFunction
return true;
}
else if (arguments.Length == 2)
Expand All @@ -208,6 +211,7 @@ private static bool TryGetCastFunction(Type genericType, bool isOpenGeneric, Typ
{
castFunction = new ExportFactoryCreator(genericType).CreateStronglyTypedExportFactoryFactory(arguments[0].UnderlyingSystemType, arguments[1].UnderlyingSystemType);
}
// castFunction
return true;
}
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,11 @@ public override ComposablePart CreatePart()
return null;
}

internal override bool TryGetExports(ImportDefinition definition, out Tuple<ComposablePartDefinition, ExportDefinition>? singleMatch, out IEnumerable<Tuple<ComposablePartDefinition, ExportDefinition>>? multipleMatches)
internal override bool TryGetExports(ImportDefinition definition, [NotNullWhen(true)] out Tuple<ComposablePartDefinition, ExportDefinition>? singleMatch, out IEnumerable<Tuple<ComposablePartDefinition, ExportDefinition>>? multipleMatches)
{
if (this.IsGeneric())
{
singleMatch = null;
singleMatch = null!; // TODO2
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 the [NotNullWhen(...)] annotation on method seems incorrect #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

Ditto #Resolved

multipleMatches = null;

List<Tuple<ComposablePartDefinition, ExportDefinition>>? exports = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public static int ListHashCode<T>(this ReadOnlyCollection<T> list)
int h = 6551;
foreach (T t in list)
{
h ^= (h << 5) ^ cmp.GetHashCode(t);
h ^= (h << 5) ^ cmp.GetHashCode(t!);
}
return h;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ internal bool HasHandler(InterpretedFrame frame, Exception exception, [NotNullWh
// Unreachable.
// Want to assert that this case isn't hit, but an assertion failure here will be eaten because
// we are in an exception filter. Therefore return true here and assert in the catch block.
handler = null;
handler = null!;
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
unwrappedException = exception;
return true;
}
Expand Down Expand Up @@ -215,9 +215,9 @@ internal sealed class DebugInfo
private class DebugInfoComparer : IComparer<DebugInfo>
{
//We allow comparison between int and DebugInfo here
int IComparer<DebugInfo>.Compare(DebugInfo d1, DebugInfo d2)
int IComparer<DebugInfo>.Compare(DebugInfo? d1, DebugInfo? d2)
{
if (d1.Index > d2.Index) return 1;
if (d1!.Index > d2!.Index) return 1; // TODO2
Copy link
Member Author

@jcouv jcouv Feb 11, 2020

Choose a reason for hiding this comment

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

📝 IComparer declares that it can compare null inputs, but this implementation doesn't handle. #Resolved

Copy link
Member

@stephentoub stephentoub Feb 11, 2020

Choose a reason for hiding this comment

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

else if (d1.Index == d2.Index) return 0;
else return -1;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ internal int GetHashCode(TInputOutput element)
(HashCodeMask &
(_elementComparer == null ?
(element == null ? NULL_ELEMENT_HASH_CODE : element.GetHashCode()) :
_elementComparer.GetHashCode(element)))
_elementComparer.GetHashCode(element!)))
% _distributionMod;
}

Expand All @@ -103,7 +103,7 @@ internal int GetHashCode(THashKey key)
(HashCodeMask &
(_keyComparer == null ?
(key == null ? NULL_ELEMENT_HASH_CODE : key.GetHashCode()) :
_keyComparer.GetHashCode(key))) % _distributionMod;
_keyComparer.GetHashCode(key!))) % _distributionMod;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ internal ForAllEnumerator(QueryOperatorEnumerator<TInput, TKey> source, Action<T
// element action for each element.
//

internal override bool MoveNext([MaybeNull, AllowNull] ref TInput currentElement, ref int currentKey)
internal override bool MoveNext([MaybeNullWhen(false), AllowNull] ref TInput currentElement, ref int currentKey)
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
{
Debug.Assert(_elementAction != null, "expected a compiled operator");

Expand All @@ -159,14 +159,13 @@ internal override bool MoveNext([MaybeNull, AllowNull] ref TInput currentElement
TInput element = default(TInput)!;
TKey keyUnused = default(TKey)!;
int i = 0;
while (_source.MoveNext(ref element!, ref keyUnused))
while (_source.MoveNext(ref element, ref keyUnused))
{
if ((i++ & CancellationState.POLL_INTERVAL) == 0)
CancellationState.ThrowIfCanceled(_cancellationToken);
_elementAction(element);
}


return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ private int GetKeyHashCode(TKey key)
return HashCodeMask &
(comparer == null ?
(key == null ? 0 : key.GetHashCode()) :
comparer.GetHashCode(key));
comparer.GetHashCode(key!));
}

private bool AreKeysEqual(TKey key1, TKey key2)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;

namespace System.Linq.Parallel
{
Expand All @@ -26,7 +27,7 @@ internal ReverseComparer(IComparer<T> comparer)
_comparer = comparer;
}

public int Compare(T x, T y)
public int Compare([AllowNull] T x, [AllowNull] T y)
{
return _comparer.Compare(y, x);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public bool Equals(Wrapper<T> x, Wrapper<T> y)
public int GetHashCode(Wrapper<T> x)
{
Debug.Assert(_comparer != null);
return _comparer.GetHashCode(x.Value);
return _comparer.GetHashCode(x.Value!);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ private EmptyPartition()
public bool MoveNext() => false;

[ExcludeFromCodeCoverage] // Shouldn't be called, and as undefined can return or throw anything anyway.
[MaybeNull]
public TElement Current => default!;

[ExcludeFromCodeCoverage] // Shouldn't be called, and as undefined can return or throw anything anyway.
Expand Down
4 changes: 3 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/Action.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;

namespace System
{
public delegate void Action();
Expand All @@ -24,7 +26,7 @@ namespace System
public delegate TResult Func<in T1, in T2, in T3, in T4, in T5, in T6, in T7, out TResult>(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5, T6 arg6, T7 arg7);
public delegate TResult Func<in T1, in T2, in T3, in T4, in T5, in T6, in T7, in T8, out TResult>(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5, T6 arg6, T7 arg7, T8 arg8);

public delegate int Comparison<in T>(T x, T y);
public delegate int Comparison<in T>([AllowNull] T x, [AllowNull] T y);
stephentoub marked this conversation as resolved.
Show resolved Hide resolved

public delegate TOutput Converter<in TInput, out TOutput>(TInput input);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public ComparisonComparer(Comparison<T> comparison)
_comparison = comparison;
}

public override int Compare(T x, T y)
public override int Compare([AllowNull] T x, [AllowNull] T y)
{
return _comparison(x, y);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace System.Collections.Generic
{
// The generic IEqualityComparer interface implements methods to if check two objects are equal
// and generate Hashcode for an object.
// It is use in Dictionary class.
// It is used in Dictionary class.
public interface IEqualityComparer<in T>
{
bool Equals([AllowNull] T x, [AllowNull] T y);
Expand Down
Loading