Adjust type of out var based on parameter state#36284
Conversation
| ns4 /*T:string?*/ .ToString(); // 6 | ||
| ns4 = null; | ||
|
|
||
| FalseOut(out var s5); |
There was a problem hiding this comment.
out var s5 [](start = 17, length = 10)
📝 This var used to be oblivious (picking its type from the TypeWithAnnotations of the out parameter). But now it gets its type from the null-state of the output value (which is non-null since the parameter is marked as oblivious). #Resolved
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
| using System.Diagnostics.CodeAnalysis; | ||
| public class Optional<T> | ||
| { | ||
| public static bool MaybeNullWhenFalse([MaybeNullWhen(false)] string s) => throw null!; |
There was a problem hiding this comment.
What does this static method do in the test? #Resolved
| // ns4 /*T:string?*/ .ToString(); // warn 6 | ||
| // ns4 /*T:string?*/ .ToString(); // 6 | ||
| Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "ns4").WithLocation(30, 9), | ||
| // (35,14): warning CS8600: Converting null literal or possible null value to non-nullable type. |
There was a problem hiding this comment.
// (35,14): warning CS8600: Converting null literal or possible null value to non-nullable type. [](start = 16, length = 96)
This warning feels unexpected. The goal of the change was to relax the type in some scenarios, but we ended up tightening the type in unrelated scenarios. #Closed
There was a problem hiding this comment.
I left a note above to explain this behavior. This is the same behavior we get from a var declaration initialized with a string~. The null-state of the value is not-null, so the var is a string!.
I think this is correct.
In reply to: 293147949 [](ancestors = 293147949)
There was a problem hiding this comment.
The null-state of the value is not-null, so the var is a string!. I think this is correct.
I cannot say that I like this behavior.
In reply to: 293634127 [](ancestors = 293634127,293147949)
There was a problem hiding this comment.
I cannot say that I like this behavior.
I agree it doesn't feel great, but I don't have a good idea for an alternative that would make sense.
Just to clarify, do you also dislike the behavior in var x = ...; scenario?
Let's file an issue if you have a suggestion for better behavior.
In reply to: 293894338 [](ancestors = 293894338,293634127,293147949)
| using System.Diagnostics.CodeAnalysis; | ||
| public class Optional<T> | ||
| { | ||
| public static bool MaybeNullWhenFalse([MaybeNullWhen(false)] string s) => throw null!; |
There was a problem hiding this comment.
public static bool MaybeNullWhenFalse([MaybeNullWhen(false)] string s) => throw null!; [](start = 4, length = 86)
This method doesn't look used. #Closed
| } | ||
|
|
||
| [Fact] | ||
| public void MaybeNullWhenFalse_TryGetValue() |
There was a problem hiding this comment.
MaybeNullWhenFalse_TryGetValue [](start = 20, length = 30)
Consider adding similar test for [MaybeNull]. #Closed
|
Done with review pass (iteration 1) #Closed |
|
@AlekseyTs I've resolved or replied to your feedback. Please take another look when you have some time. Thanks |
…-types * dotnet/master: (63 commits) Fix stack overflow in requesting syntax directives (dotnet#36347) crash on ClassifyUpdate for EventFields (dotnet#35962) Disable move type when the options service isn't present (dotnet#36334) Fix crash where type inference doing method inference needs to drop nullability Fix parsing bug in invalid using statements (dotnet#36428) Do not suggest or diagnose use compound assignment when right hand of binary operator is a throw expression Add option to emit nullable metadata for public members only (dotnet#36398) Added null checks on F# external access services (dotnet#36469) Deal with discovering extra .editorconfig files Re-enable MSBuildWorkspaceTests.TestEditorConfigDiscovery Add support to VisualStudioMSBuildInstalled to support minimum versions Fix configuration of accessibilities in editorconfig Shorten a resource ID Revert "Extract the RDT implementation for Misc files and VS open file tracker" Add nullability support to use local function Add EditorFeatures.WPF dependency to F# ExternalAccess Ensure NullableWalker.AsMemberOfType locates the right new container for the member. (dotnet#36406) Replace `dynamic` with `object` when substituting constraints. (dotnet#36379) Add some string descriptions Adjust type of out var based on parameter state (dotnet#36284) ...
This PR fixes the problem with
TryGetValue(key, out var value). Previously we would have warned on output assignment tovalue. Now we adjust the type ofvarso that there is no warning.In
var x = MaybeNull(y);we use the null-state of the return value to determine the type ofvar. If the return value is annotated with[MaybeNull], the null-state will be maybe-null even if the inference type arguments for the method are un-annotated, and the type of thevarwill be annotated.Similarly, we want the null-state of the out parameters to help determine the type of out vars. So
MaybeNull(y, out var x)can result in avarwith an annotated type.Relates to #35816 (list of work items related to nullable annotation attributes)