Initial support for [AllowNull], [DisallowNull], [MaybeNull], [NotNull] in NullableWalker#35955
Initial support for [AllowNull], [DisallowNull], [MaybeNull], [NotNull] in NullableWalker#35955jcouv merged 9 commits intodotnet:masterfrom
Conversation
| NullableFlowState state; | ||
| if (type.CanContainNull()) | ||
| { | ||
| if ((annotations & FlowAnalysisAnnotations.MaybeNull) != 0) |
There was a problem hiding this comment.
Did LDM confirm that MaybeNull takes precedence over NotNull? #Resolved
There was a problem hiding this comment.
This seems sensible (conservative/safe to return the worst case state).
I took a note in dotnet/csharplang#2201 to let LDM know.
In reply to: 288212991 [](ancestors = 288212991)
| @"using System.Runtime.CompilerServices; | ||
| class Program | ||
| { | ||
| [return: MaybeNull] static T F1<T>(T t) => t; |
There was a problem hiding this comment.
Can we add some tests where a user specifies both MaybeNull and NotNull, and show the behavior? #Resolved
| var type = typeWithAnnotations.Type; | ||
| if (type is null) | ||
| { | ||
| return new TypeWithState(null, NullableFlowState.MaybeNull); |
There was a problem hiding this comment.
return new TypeWithState(null, NullableFlowState.MaybeNull); [](start = 16, length = 60)
📝 This line is not covered. Not sure if it could be. #Closed
| } | ||
| else | ||
| { | ||
| state = typeWithAnnotations.NullableAnnotation switch |
There was a problem hiding this comment.
typeWithAnnotations.NullableAnnotation switch [](start = 28, length = 45)
📝 I expected we'd return typeWithAnnotations.ToTypeWithState() here. #Closed
| } | ||
| static void M2(int? i2) | ||
| { | ||
| F2(i2); |
There was a problem hiding this comment.
F2(i2); [](start = 8, length = 7)
I expected a warning here (we're passing a maybe-null state into a parameter marked with [DisallowNull]). Or maybe the warning is disallowing this attribute on non-generic types...
Filed #36009 #Resolved
| } | ||
| } | ||
|
|
||
| public TypeWithAnnotations ApplyLValueAnnotations(TypeWithAnnotations declaredType, FlowAnalysisAnnotations flowAnalysisAnnotations = FlowAnalysisAnnotations.None) |
There was a problem hiding this comment.
private static #Closed
|
b5f348d LGTM |
|
@dotnet/roslyn-compiler for review. |
| F1(x).ToString(); // 2 | ||
| F1(y).ToString(); | ||
| F2(x).ToString(); // 3 | ||
| F2(y).ToString(); // 4 |
There was a problem hiding this comment.
// 4 [](start = 25, length = 5)
📝 incorrect comment actually, missing diagnostic #Resolved
There was a problem hiding this comment.
I'm not sure I agree. The attribute was meaningless where written, as the return type could have been written T?. So I think it is safe to ignore it in analysis.
In reply to: 288763015 [](ancestors = 288763015)
There was a problem hiding this comment.
Having said that, we should have more tests for reading from assembly images with meaningful annotations.
In reply to: 289210231 [](ancestors = 289210231,288763015)
| // x1.ToString(); // 1 | ||
| Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x1").WithLocation(11, 9), | ||
| // (12,9): warning CS8602: Dereference of a possibly null reference. | ||
| // y1.ToString(); |
There was a problem hiding this comment.
y1.ToString(); [](start = 27, length = 14)
📝 I'm investigating this. I'll push a commit to fix.
There was a problem hiding this comment.
I believe this is correct. The argument type and state is object?, so that is what we infer for T. Then we report the constraint failure. Since we don't use constraints in type inference, I believe this is correct.
In reply to: 288805395 [](ancestors = 288805395)
| { | ||
| var type = typeWithAnnotations.Type; | ||
| if (type is null) | ||
| { |
There was a problem hiding this comment.
I don't understand the purpose of this if statement. If it isn't necessary, please remove it. #Resolved
I think there should be a warning here, as this attribute doesn't actually allow anything (see errors on calls to Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:22170 in 575bdf4. [](commit_id = 575bdf4, deletion_comment = False) |
You're correct. The problem with loading from PE is only for the return value. It's a simple fix, so I'll include in this PR (next commit). In reply to: 497494525 [](ancestors = 497494525) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:22791 in 575bdf4. [](commit_id = 575bdf4, deletion_comment = False) |
|
Generally looks good, but I think it would be good to have warnings where the attributes make no sense or could be expressed directly in annotations. It would be good to have a couple of cross-compilation tests. If you want to defer that by adding issues tracking that additional work I'd be OK with this as is (Iteration 6) #Resolved |
|
|
||
| private static TypeWithAnnotations ApplyRValueAnnotations(TypeWithAnnotations declaredType, FlowAnalysisAnnotations flowAnalysisAnnotations) | ||
| { | ||
| if ((flowAnalysisAnnotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull) |
There was a problem hiding this comment.
== FlowAnalysisAnnotations.NotNull [](start = 76, length = 34)
The comparison style should be consistent between this method and the one above. #Resolved
There was a problem hiding this comment.
I'll change, but the reason for this comparison style is that NotNull is two bits, whereas DisallowNull is only one bit (so simpler comparison works ok there)
In reply to: 289486556 [](ancestors = 289486556)
| else if ((flowAnalysisAnnotations & FlowAnalysisAnnotations.MaybeNull) == FlowAnalysisAnnotations.MaybeNull) | ||
| { | ||
| var typeSymbol = declaredType.Type; | ||
| if (!declaredType.NullableAnnotation.IsNotAnnotated() || (typeSymbol.IsValueType && typeSymbol.IsNullableType())) |
There was a problem hiding this comment.
!declaredType.NullableAnnotation.IsNotAnnotated() [](start = 20, length = 49)
declaredType.NullableAnnotation.IsAnnotated() #Resolved
| return declaredType; | ||
| } | ||
|
|
||
| return TypeWithAnnotations.Create(typeSymbol, NullableAnnotation.NotAnnotated, declaredType.CustomModifiers); |
There was a problem hiding this comment.
Consider extracting helper methods to share code between this method and the one above. For instance, this could be return AsNotAnnotated(declaredType); #Resolved
|
1ab07a0 LGTM |
|
Iteration 9 LGTM #Closed |
Some support for simple pre- and post-condition nullable annotations on by-value parameters and return types.
Added to this PR:
A number of scenarios and tests will be covered in later PRs:
ReturnTypeWithAnnotationsinNullableWalker)