-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
React to latest Roslyn nullability changes #36104
React to latest Roslyn nullability changes #36104
Conversation
|
||
if (waitForSemaphoreWasSuccessful) | ||
{ | ||
Debug.Assert(item != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open question: I'm not sure if this assertion is actually correct. Is it possible to have - say - BlockingCollection<string?>
? If that's legal, we should remove this assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have - say -
BlockingCollection<string?>
?
Yes, that's valid. Please revert the added code. It's a little disheartening this didn't cause any existing tests to fail. Sigh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove the assertion and add an explicit unit test.
@@ -354,7 +354,7 @@ public abstract partial class Array : System.Collections.ICollection, System.Col | |||
public static int LastIndexOf<T>(T[] array, T value) { throw null; } | |||
public static int LastIndexOf<T>(T[] array, T value, int startIndex) { throw null; } | |||
public static int LastIndexOf<T>(T[] array, T value, int startIndex, int count) { throw null; } | |||
public static void Resize<T>([System.Diagnostics.CodeAnalysis.NotNullAttribute] ref T[]? array, int newSize) { } | |||
public static void Resize<T>([System.Diagnostics.CodeAnalysis.NotNullAttribute] ref T[]? array, int newSize) { throw null; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting consequence of the recent [NotNull]
change is that the compiler now treats it as an out-like parameter: you must assign a non-null value to it before method exit. So this means our ref assemblies' dummy method bodies need to throw, just as they would for any other method which has an out parameter.
I didn't see any other cases where this needed to be changed, but might have implications for our "generate ref asms automatically" tool.
@steveharter @layomia @jozkee Can I get your eyes specifically on the commit 29a837b in this PR? There was a bit to untangle and I was trying to follow other established patterns within System.Text.Json. |
return waitForSemaphoreWasSuccessful; | ||
#pragma warning restore CS8762 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no way to suppress this warning with a well-placed !
somewhere? The clutter caused by !
is bad enough, but adding a bunch of pragmas to suppress false positive nullable warnings just makes me sad.
cc: @jcouv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still not great, but possibly a bit better: add a return true;
below the Debug.Assert(item != null);
at line 761.
From the language/compiler perspective, it may be useful to allow return waitForSemaphoreWasSuccessful!;
to solve this.
In reply to: 422141700 [](ancestors = 422141700)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the language/compiler perspective, it may be useful to allow return waitForSemaphoreWasSuccessful!; to solve this.
@jcouv, yeah, let's do that 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider using the if (condition) { assert(); return true; } else { return false; }
pattern, but it produces different IL and codegen than a simple return condition;
. I tried to avoid making changes that affected the codegen in any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, I missed that the Debug.Assert(item != null)
doesn't work here. An alternative would be _ = item!;
to pretend that it's not null, but that smells in this case, and also that's not yet implemented.
I'll push on allowing suppression on return someBoolValue!;
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option (also smells) is to suppress on places that assign to item
(pretend like we're never assigning maybe-null values)
In reply to: 422242826 [](ancestors = 422242826)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, using the ! operator inappropriately early in the method strikes me as undesirable. I'll leave the pragma for now and add a code comment directing to #36132. Once the language team settles on guidance for this we can react to it here.
Much appreciate your responsiveness on this! :)
...raries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentBag.cs
Show resolved
Hide resolved
....Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing these up.
The System.Text.Json changes have been addressed in #35826, in reaction to All things considered, after that PR is merged (which should be very soon), there shouldn't be any changes to System.Text.Json needed here. FWIW the System.Text.Json changes LGTM, so I'm also fine with this PR going in first, with #35826 reacting to the change. |
@@ -97,7 +97,9 @@ public partial class WebHeaderCollection : System.Collections.Specialized.NameVa | |||
public void Add(System.Net.HttpRequestHeader header, string? value) { } | |||
public void Add(System.Net.HttpResponseHeader header, string? value) { } | |||
public void Add(string header) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these presumably complicate things next time we run the tool to update the refs.
Thanks @layomia for the tip! If your stuff is going in today I'll back my JSON changes out and rebase on top of yours when ready. |
@GrabYourPitchforks #35826 is in now (sorry for conflicts ;) |
Conflicts are fine. I appreciate your efforts @layomia over on the JSON + nullable PR and for driving it through API review! :) |
29a837b
to
9a70a2b
Compare
PR feedback-inspired changes since last iteration:
|
Follow-up to #35782, which addressed the corelib side of things. This PR addresses the libraries side.
Due to #34417 (comment) I can't remove the override in Versions.props just yet. But the latest nightly compiler toolchain fixes that blocking issue, so this PR updates the explicit override to point to the latest nightly to demonstrate that we do compile cleanly against it.
I plan on reverting my changes to Versions.props as part of checkin, assuming the updated code also compiles cleanly on the older toolset. If the updated code doesn't compile cleanly on the older toolset, then the best course of action might be to wait until a new drop of Arcade comes out which picks up the updated compiler, then update Arcade, merge the contents of this PR, and remove the override from Versions.props all in one large PR.
Contributes to #34417.