Fix/76886 await nullable value type#82146
Conversation
|
@dotnet-policy-service agree |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM aside from suggestion to delete HasErrors check.
Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
|
@dotnet/roslyn-compiler for second review |
src/Compilers/CSharp/Test/Semantic/Semantics/NullableAwaitTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/NullableAwaitTests.cs
Outdated
Show resolved
Hide resolved
|
It looks like the change revealed legitimate new nullable warnings in the compiler source code. Here are my recommendations for what to try to resolve the warnings diff --git a/src/EditorFeatures/Test/InheritanceMargin/InheritanceMarginTests.cs b/src/EditorFeatures/Test/InheritanceMargin/InheritanceMarginTests.cs
index f4650be4a5a..8806c2c248e 100644
--- a/src/EditorFeatures/Test/InheritanceMargin/InheritanceMarginTests.cs
+++ b/src/EditorFeatures/Test/InheritanceMargin/InheritanceMarginTests.cs
@@ -167,6 +167,7 @@ private static async Task VerifyInheritanceTargetAsync(Workspace workspace, Test
for (var i = 0; i < actualDocumentSpans.Length; i++)
{
var docSpan = await actualDocumentSpans[i].TryRehydrateAsync(workspace.CurrentSolution, CancellationToken.None);
+ Assert.True(docSpan.HasValue);
Assert.Equal(expectedDocumentSpans[i].SourceSpan, docSpan.Value.SourceSpan);
Assert.Equal(expectedDocumentSpans[i].Document.FilePath, docSpan.Value.Document.FilePath);
}
diff --git a/src/Workspaces/Core/Portable/Rename/IRemoteRenamerService.cs b/src/Workspaces/Core/Portable/Rename/IRemoteRenamerService.cs
index 1b4348b70c3..f0984a8ae0d 100644
--- a/src/Workspaces/Core/Portable/Rename/IRemoteRenamerService.cs
+++ b/src/Workspaces/Core/Portable/Rename/IRemoteRenamerService.cs
@@ -136,7 +136,7 @@ internal sealed partial class SymbolicRenameLocations
serializableLocations.Options,
locations,
implicitLocations,
- referencedSymbols);
+ referencedSymbols!);
}
}
diff --git a/src/Workspaces/Core/Portable/Rename/LightweightRenameLocations.cs b/src/Workspaces/Core/Portable/Rename/LightweightRenameLocations.cs
index ad9b98cce7f..8891be76f91 100644
--- a/src/Workspaces/Core/Portable/Rename/LightweightRenameLocations.cs
+++ b/src/Workspaces/Core/Portable/Rename/LightweightRenameLocations.cs
@@ -63,7 +63,7 @@ internal sealed partial class LightweightRenameLocations
Options,
Locations,
implicitLocations,
- referencedSymbols);
+ referencedSymbols!);
}
/// <summary> |
|
@davidwengier there is a pretty good chance that when we take this change, downstream projects will get new nullable warnings. Just a heads up |
|
Finally got a test insertion going, am seeing new nullable warnings and pushing commits to address, to try and get the VS build all the way through and hopefully see the extent of the impact. https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/713230 Will want to do the same in dotnet/runtime. Have an agent following the steps to validate changes in dotnet/runtime and will see what comes up when I get back to investigating it sometime this week. |
…lable-value-type # Conflicts: # src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
|
Verified VS and dotnet/runtime builds. There were just a few new test warnings in VS and no new warnings in dotnet/runtime. |
Co-authored-by: Metalaka <Metalaka@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Should we add an entry to our breaking change doc?
There was a problem hiding this comment.
To me, it's just under the bar. We have had nullable bugs that impacted accuracy before, and, in most cases never added entries to the breaking change doc for them.
However, I do know we have some deadlines for 3xx to make this week, so, to avoid potential disruption I think we should wait for the next band.
|
Thanks so much @RikkiGibson for the great review and guidance on this 🙌 💖 |
|
@Metalaka thanks for the contribution! |
Summary
Fixes a bug where
awaiton aNullable<T>(e.g.,Task<int?>) was incorrectly treated asNotNullby the nullable flow analysis.Task<int?>resulted in aNotNullflow state, causing the compiler to miss warnings when the result was potentially null.GetResultmethod.Description
In
NullableWalker.VisitAwaitExpression, the compiler previously assumed that anyawaitresult of aValueTypewas inherently non-null.This PR updates
VisitAwaitExpressionto explicitly check if the type is aNullable<T>before deciding to mark it asNotNull. If it is aNullable<T>, the flow analysis now correctly uses the nullability information from theGetResultmethod of the awaiter.Impact on Existing Code
By correctly identifying the result of
await Task<T?>as potentially null, this change will introduce new CS8629 (Nullable value type may be null) warnings in existing code where these potential nulls were previously ignored by the flow analysis. This is intended behavior to ensure correct nullable safety.Fixed Issues
Fixes #76886
Tests Added
Added new test cases to
NullableReferenceTypesTests.cs:Regression Tests (validating the fix for #76886):
AwaitedNullableValueTypeAssignedToNonNullable_ProducesCS8629AwaitResultNullableValueTypeAssignedToNonNullable_ProducesCS8629Baseline Tests (validating actual/expected behavior for non-awaited types):
AwaitedNullableTypeAssignedToNonNullable_ProducesCS8600AwaitResultNullableTypeAssignedToNonNullable_ProducesCS8600NullableValueTypeAssignedToNonNullable_ProducesCS8629