Fix/76886 await nullable value type #82146
Conversation
dotnet-policy-service
bot
added
the
Community
|
@dotnet-policy-service agree |
RikkiGibson
left a comment
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 |
| var comp = CreateCompilation(src); | ||
| comp.VerifyDiagnostics( | ||
| // (11,30): warning CS8629: Nullable value type may be null. | ||
| // int nonNullableInt = result.Value; // warn expected |
There was a problem hiding this comment.
nit: this comment doesn't correspond to the original source (there is no // warn expected there).
This applies to a few other places in this file too
| // (11,36): warning CS8600: Converting null literal or possible null value to non-nullable type. | ||
| // string nonNullableString = result; // warn | ||
| Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "result") | ||
| .WithLocation(11, 36)); |
There was a problem hiding this comment.
Consider leaving the diagnostics formatted exactly as they come from the test output - i.e., with the .WithLocation on the same line as Diagnostic(. To keep the style consistent but also simpler to update if diagnostics change in the future (and resulting in smaller diffs).
There was a problem hiding this comment.
It's now done for the entire file, Thanks
|
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 |
|
/pr-val d847a89 |
|
View PR Validation Run triggered by @RikkiGibson Parameters
|
3 participants
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