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

Address warnings for possibly null array elements #41046

Merged
merged 3 commits into from
Aug 20, 2020
Merged

Address warnings for possibly null array elements #41046

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cb3b76
Suppress warnings for possibly null array elements
cston Aug 19, 2020
7f485cc
Add [AllowNull]
cston Aug 19, 2020
cf4ecb4
Add [AllowNull]
cston Aug 19, 2020
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: 4 additions & 4 deletions src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,8 +473,8 @@ private static int PickPivotAndPartition(Span<T> keys)
{
if (pivot == null)
{
while (Unsafe.IsAddressLessThan(ref leftRef, ref nextToLastRef) && (leftRef = ref Unsafe.Add(ref leftRef, 1)) == null) ;
while (Unsafe.IsAddressGreaterThan(ref rightRef, ref zeroRef) && (rightRef = ref Unsafe.Add(ref rightRef, -1)) == null) ;
while (Unsafe.IsAddressLessThan(ref leftRef!, ref nextToLastRef) && (leftRef = ref Unsafe.Add(ref leftRef, 1)) == null) ;
while (Unsafe.IsAddressGreaterThan(ref rightRef!, ref zeroRef) && (rightRef = ref Unsafe.Add(ref rightRef, -1)) == null) ;
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 not understanding why the compiler flags this. Why does the compiler think these methods could assign null to the ref?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the warning is for the value of ref argument when the method is called. On the second time through the loop, leftRef is assumed to be null based on && (leftRef = ...) == null.

Copy link
Member

@stephentoub stephentoub Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Is the right fix here then to actually annotate both arguments with [AllowNull]? (That would apply to all members of Unsafe that accept multiple ref Ts and only care about the address, not the actual value, e.g. AreSame, ByteOffset, IsAddressGreaterThan, IsAddressLessThan, and in both the public Unsafe.cs ref as well as the internal implementation in corelib.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the right fix here then to actually annotate both arguments with [AllowNull]?

The methods in Unsafe simply take an unconstrained T which seems like it should be sufficient for all callers. Should the leftRef, etc. locals be declared as ref T? here instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should the leftRef, etc. locals be declared as ref T? here instead?

Perhaps not. That would just move the warnings to the initializers of those locals.

Copy link
Member

Choose a reason for hiding this comment

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

Should the leftRef, etc. locals be declared as ref T? here instead?

Wouldn't that be a problem if a T t; were passed in, with the compiler then complaining that the implementation might assign null to it?

Copy link
Member

@stephentoub stephentoub Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should the leftRef, etc. locals be declared as ref T? here instead?

Perhaps not. That would just move the warnings to the initializers of those locals.

Right, hence my question about [AllowNull]. Does that make sense?

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 similar to the Volatile.Read APIs, where ideally we could change ref T to in T, but that would be a breaking API change.

}
else
{
Expand Down Expand Up @@ -552,7 +552,7 @@ private static void InsertionSort(Span<T> keys)
j--;
}

Unsafe.Add(ref MemoryMarshal.GetReference(keys), j + 1) = t;
Unsafe.Add(ref MemoryMarshal.GetReference(keys), j + 1) = t!;
}
}

Expand Down Expand Up @@ -1054,7 +1054,7 @@ private static void InsertionSort(Span<TKey> keys, Span<TValue> values)
j--;
}

keys[j + 1] = t;
keys[j + 1] = t!;
values[j + 1] = tValue;
}
}
Expand Down