Report bad tightening/loosening of attributes in OHI #41336
Conversation
jcouv
added
Area-Compilers
New Language Feature - Nullable Reference Types
| @@ -108,3 +108,16 @@ Similarly, a value from a `[DisallowNull] ref string? p` parameter will be assum | |||
| On the other hand, we'll warn for returning a `null` from a method declared with `[NotNull] string?` return type, and we'll treat the value from a `[AllowNull] ref string p` parameter as maybe-null. | |||
| Conditional attributes are treated leniently. For instance, no warning will be produced for assigning a maybe-null value to an `[MaybeNullWhen(...)] out string p` parameter. | |||
|
|
|||
| 19. https://github.com/dotnet/roslyn/issues/36039 In *Visual Studio 2019 version 16.5* and onwards, the compiler did not check the usage of nullable flow annotation attributes, such as `[MaybeNull]` or `[NotNull]`, in overrides or implementations. In *Visual Studio 2019 version 16.5*, those usages are checked to respect null discipline. For example: | |||
There was a problem hiding this comment.
| 19. https://github.com/dotnet/roslyn/issues/36039 In *Visual Studio 2019 version 16.5* and onwards, the compiler did not check the usage of nullable flow annotation attributes, such as `[MaybeNull]` or `[NotNull]`, in overrides or implementations. In *Visual Studio 2019 version 16.5*, those usages are checked to respect null discipline. For example: | |
| 19. https://github.com/dotnet/roslyn/issues/36039 Prior to *Visual Studio 2019 version 16.5*, the compiler did not check the usage of nullable flow annotation attributes, such as `[MaybeNull]` or `[NotNull]`, in overrides or implementations. In *Visual Studio 2019 version 16.5* and onwards, those usages are checked to respect null discipline. For example: | |
| ``` #Resolved |
| The use of any member returning `[MaybeNull]T` for an unconstrained type parameter `T` produces a warning, just like `default(T)` (see below). For instance, `listT.FirstOrDefault();` would produce a warning. | ||
|
|
||
| Enforcing nullability attributes within method bodies: | ||
| - An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state. |
There was a problem hiding this comment.
| - An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state. | |
| - An input parameter marked with `[AllowNull]` is initialized with a maybe-null (or maybe-default) state. | |
| ``` #Resolved |
|
|
||
| Enforcing nullability attributes within method bodies: | ||
| - An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state. | ||
| - An input parameter marked with [DisallowNull] is initialized with a not-null state. |
There was a problem hiding this comment.
| - An input parameter marked with [DisallowNull] is initialized with a not-null state. | |
| - An input parameter marked with `[DisallowNull]` is initialized with a not-null state. | |
| ``` #Resolved |
| Enforcing nullability attributes within method bodies: | ||
| - An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state. | ||
| - An input parameter marked with [DisallowNull] is initialized with a not-null state. | ||
| - A parameter marked with [MaybeNull] or [MaybeNullWhen] can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with [NotNullWhen] (the attribute is ignored). |
There was a problem hiding this comment.
| - A parameter marked with [MaybeNull] or [MaybeNullWhen] can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with [NotNullWhen] (the attribute is ignored). | |
| - A parameter marked with `[MaybeNull]` or `[MaybeNullWhen]` can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with `[NotNullWhen]` (the attribute is ignored). | |
| ``` #Resolved |
| - An input parameter marked with [AllowNull] is initialized with a maybe-null (or maybe-default) state. | ||
| - An input parameter marked with [DisallowNull] is initialized with a not-null state. | ||
| - A parameter marked with [MaybeNull] or [MaybeNullWhen] can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with [NotNullWhen] (the attribute is ignored). | ||
| - A parameter marked with [NotNull] will produce a warning when assigned a maybe-null value. Same for return values. |
There was a problem hiding this comment.
This only applies to out and ref parameters right?
| - A parameter marked with [NotNull] will produce a warning when assigned a maybe-null value. Same for return values. | |
| - A by-reference parameter marked with `[NotNull]` will produce a warning when assigned a maybe-null value. Same for return values. | |
| ``` #Resolved |
| - An input parameter marked with [DisallowNull] is initialized with a not-null state. | ||
| - A parameter marked with [MaybeNull] or [MaybeNullWhen] can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with [NotNullWhen] (the attribute is ignored). | ||
| - A parameter marked with [NotNull] will produce a warning when assigned a maybe-null value. Same for return values. | ||
| - The state of a parameter marked with [Maybe/NotNullWhen] is checked upon exiting the method instead. |
There was a problem hiding this comment.
| - The state of a parameter marked with [Maybe/NotNullWhen] is checked upon exiting the method instead. | |
| - The state of a parameter marked with `[MaybeNullWhen]`/`[NotNullWhen]` is checked upon exiting the method instead. | |
| ``` #Resolved |
| - The state of a parameter marked with [Maybe/NotNullWhen] is checked upon exiting the method instead. | ||
| - A method marked with [DoesNotReturn] will produce a warning if it returns or exits normally. | ||
|
|
||
| Note: we don't validate the internal consistency of auto-properties, so it is possible to misused attributes on auto-props as on fields. For example: `[AllowNull, NotNull] public TOpen P { get; set; }`. |
There was a problem hiding this comment.
💡 Consider breaking out a section specifically to point out things that are not checked #WontFix
|
|
||
| Enforcing nullability attributes when overriding and implementing: | ||
| In addition to checking the types in overrides/implementations are compatible with overridden/implemented members, we also check that the nullability attributes are compatible. | ||
| - For input parameters (by-value and `in`), we check that the worst allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter. |
There was a problem hiding this comment.
| - For input parameters (by-value and `in`), we check that the worst allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter. | |
| - For input parameters (by-value and `in`), we check that the widest allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter. | |
| ``` #Resolved |
| Enforcing nullability attributes when overriding and implementing: | ||
| In addition to checking the types in overrides/implementations are compatible with overridden/implemented members, we also check that the nullability attributes are compatible. | ||
| - For input parameters (by-value and `in`), we check that the worst allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter. | ||
| - For output parameters (`out` and return values), we check that the worst produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter. |
There was a problem hiding this comment.
| - For output parameters (`out` and return values), we check that the worst produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter. | |
| - For output parameters (`out` and return values), we check that the widest produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter. | |
| ``` #Resolved |
| - For input parameters (by-value and `in`), we check that the worst allowed value by the overridden/implemented parameter can be assigned to the overriding/implementing parameter. | ||
| - For output parameters (`out` and return values), we check that the worst produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter. | ||
| - For `ref` parameters and return values, we check both. | ||
| - We check that the post-condition contract `[Not/MaybeNull]` on input parameters is enforced by overriding/implementing members. |
There was a problem hiding this comment.
| - We check that the post-condition contract `[Not/MaybeNull]` on input parameters is enforced by overriding/implementing members. | |
| - We check that the post-condition contract `[NotNull]`/`[MaybeNull]` on input parameters is present and enforced by overriding/implementing members. | |
| ``` #Resolved |
| - For output parameters (`out` and return values), we check that the worst produced value by the overriding/implementing parameter can be assigned to the overridden/implemented parameter. | ||
| - For `ref` parameters and return values, we check both. | ||
| - We check that the post-condition contract `[Not/MaybeNull]` on input parameters is enforced by overriding/implementing members. | ||
| - We check that overrides/implementations have the `[DoesNotReturn]` attribute if the overridden/implemented member has it. |
There was a problem hiding this comment.
❔ Do we allow an implementation to use [DoesNotReturn] if the original definition used [DoesNotReturnIf]? #Resolved
There was a problem hiding this comment.
I'll add a test, but I think we allow it and that behavior is correct.
The implementation abides by the overridden/implemented method's contract.
In reply to: 373550920 [](ancestors = 373550920)
There was a problem hiding this comment.
It turns out the problem doesn't even present itself. [DoesNotReturn] is applied to methods, but [DoesNotReturnIf(...)] is applied to parameters. They don't cross paths. #Resolved
jcouv
force-pushed
the
nullable-ohi
branch
2 times, most recently
from
84385ec
to
d2194ab
Compare
| - An input parameter marked with `[DisallowNull]` is initialized with a not-null state. | ||
| - A parameter marked with `[MaybeNull]` or `[MaybeNullWhen]` can be assigned a maybe-null value, without warning. Same for return values. Same for a nullable parameter marked with `[NotNullWhen]` (the attribute is ignored). | ||
| - A parameter marked with `[NotNull]` will produce a warning when assigned a maybe-null value. Same for return values. | ||
| - The state of a parameter marked with `[MaybeNullWhen]`\`[NotNullWhen]` is checked upon exiting the method instead. |
There was a problem hiding this comment.
\ [](start = 56, length = 1)
"/" or "or" #Closed
| bool overriddenHasNotNull = (overriddenAnnotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull; | ||
| if (overriddenHasNotNull && !overridingHasNotNull && !forRef) | ||
| { | ||
| // Overriding doesn't conform to contract of overridden (ie. promise not to return if parameter is null) |
There was a problem hiding this comment.
promise not to return if parameter is null [](start = 81, length = 42)
What does "promise not to return" mean for parameters? #Closed
There was a problem hiding this comment.
void AssertNotNull<T>([NotNull] T t) would be an example. The method promises not to return if the parameter is null. #Closed
| { | ||
| // Can't assign value from overriding to overridden in true case | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Consider extracting a helper method, with sense parameter, for use here and false case below. #Closed
| } | ||
| : | ||
| (Action<DiagnosticBag, MethodSymbol, MethodSymbol, ParameterSymbol, Location>)null, | ||
| checkReturnType ? reportBadReturn : (ReportMismatchinReturnType<Location>)null, |
There was a problem hiding this comment.
(ReportMismatchinReturnType) [](start = 85, length = 38)
Are the casts needed here or for checkParameters? #Closed
There was a problem hiding this comment.
Yes, the casts are necessary. #Resolved
| } | ||
| : | ||
| (Action<DiagnosticBag, MethodSymbol, MethodSymbol, ParameterSymbol, Location>)null, | ||
| checkReturnType ? reportBadReturn : (ReportMismatchinReturnType<Location>)null, |
There was a problem hiding this comment.
reportBadReturn [](start = 67, length = 15)
The local functions will result in delegate instances. Consider inlining the local functions instead. #Closed
| reportMismatchInParameterType(diagnostics, overriddenMethod, overridingMethod, overridingParameters[i], extraArgument); | ||
| reportMismatchInParameterType(diagnostics, overriddenMethod, overridingMethod, parameter, false, extraArgument); | ||
| } | ||
| else if (reportMismatchInParameterType != null && |
There was a problem hiding this comment.
reportMismatchInParameterType != null [](start = 25, length = 37)
Already checked before the for loop. #Closed
| @@ -1104,14 +1126,25 @@ static bool isOrContainsErrorType(TypeSymbol typeSymbol) | |||
| for (int i = 0; i < overriddenMethod.ParameterCount; i++) | |||
| { | |||
| var overriddenParameterType = overriddenParameters[i].TypeWithAnnotations; | |||
There was a problem hiding this comment.
overriddenParameters[i] [](start = 46, length = 23)
Consider extracting a local. #Closed
| { | ||
| diagnostics.Add(ErrorCode.WRN_DoesNotReturnMismatch, overridingMethod.Locations[0], new FormattedSymbol(overridingMethod, SymbolDisplayFormat.MinimallyQualifiedFormat)); | ||
| } | ||
|
|
||
| var conversions = compilation.Conversions.WithNullability(true); | ||
| if (reportMismatchInReturnType != null && |
There was a problem hiding this comment.
reportMismatchInReturnType != null [](start = 16, length = 34)
Please consider checking this condition once, around this case and the one below. #Closed
| if (overriddenHasNotNull && !overridingHasNotNull && !forRef) | ||
| { | ||
| // Overriding doesn't conform to contract of overridden (ie. promise not to return if parameter is null) | ||
| return false; |
There was a problem hiding this comment.
false [](start = 27, length = 5)
Do we get here with substituted types?
abstract class A<T>
{
[return: NotNull] abstract T F();
}
class B : A<string?>
{
override string F() => string.Empty;
}
Or cases where override differs by attributes but not by actual nullability?
abstract class A
{
[return: MaybeNull] abstract string F();
}
class B : A<string?>
{
override string? F() => null;
}
``` #Closed
There was a problem hiding this comment.
We only get here with input parameters. [NotNull] takes a special meaning on inputs, namely "the method will not return if the input is null". This contract can only be represented with the [NotNull] attribute and cannot be represented with types. So we need the attribute regardless of substitution.
I'll double-check the behavior for The behavior of ref scenarios. The !forRef check might be wrong.[NotNull] ref parameter is correct. There [NotNull] strictly has it's post-condition meaning. #Resolved
Consider dropping the "_OHI" qualifier from tests if it's not needed. (Hiding is not included and the specific overriding or implementing is usually clear from the test name.) #Closed Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:20882 in 3b9d591. [](commit_id = 3b9d591, deletion_comment = False) |
| overridingType.ToTypeWithState(), | ||
| makeUnconditionalAnnotation(overridingAnnotations, sense)); | ||
|
|
||
| var destAnnotationsWhenTrue = ToInwardAnnotations(makeUnconditionalAnnotation(overriddenAnnotations, sense)); |
There was a problem hiding this comment.
ToInwardAnnotations [](start = 46, length = 19)
What does ToInwardAnnotations mean? Does it refer to annotations that affect input, e.g. AllowNull? Maybe it would be better to use the term inbound or input as it has had other uses throughout the walker and we can avoid using multiple terms to refer to the same specific thing. #Resolved
There was a problem hiding this comment.
If you have a void M([AllowNull] string x) method, inside the body NullableWalker needs to see the inwards annotation of x, ie. [MaybeNull].
That way the initial state of x will be maybe-null (since we allow null from the caller).
"Inward" is a different concept, to refer to this reversal. #Resolved
| if (refKind == RefKind.Ref) | ||
| { | ||
| // ref variables are invariant | ||
| return AreParameterAnnotationsCompatible(RefKind.None, overriddenType, overriddenAnnotations, overridingType, overridingAnnotations, forRef: true) && |
There was a problem hiding this comment.
Why do we pass in RefKind.None and also forRef: true? What distinguishes the meaning of those arguments? #Resolved
There was a problem hiding this comment.
On a RefKind.None parameter, [NotNull] takes a special meaning. We don't want that meaning to apply to a ref argument when it is analyzed as RefKind.None + RefKind.Out. #Resolved
| @@ -1157,6 +1160,121 @@ static bool isMaybeDefaultValue(TypeWithState valueType) | |||
| } | |||
| } | |||
|
|
|||
| internal static bool AreParameterAnnotationsCompatible( | |||
There was a problem hiding this comment.
Perhaps this should be named CheckOverridingParameterAnnotations? Since we tend to "blame" the overriding method for not conforming to the requirements of the overridden method? Also wondering if it might make sense to declare closer to its use site in SourceMemberContainerSymbol_ImplementationChecks. #WontFix
There was a problem hiding this comment.
I initially has this method in SourceMemberContainerSymbol_ImplementationChecks, but that required to expose multiple private methods of NulllableWalker as internal. So I figured this was a more natural place.
I'll adjust the name #Resolved
There was a problem hiding this comment.
I ended up keeping the current name because this method doesn't report any diagnostics. The caller is assigning blame to the overriding/implementing parameter ;-)
Also, "CheckXYZ" doesn't convey that it returns a boolean. So I prefer an IsXYZ or AreXYZ convention.
In reply to: 377404557 [](ancestors = 377404557)
|
Done with review pass (iteration 7) #Resolved |
|
Created PR dotnet/runtime#32090 for impact of this change on |
We are just recording the existing behavior. #Resolved |
That would make sense, but I'm not sure I have a compelling example. |
A using System.Diagnostics.CodeAnalysis;
#nullable enable
public class C
{
public void M([MaybeNull] string x)
{
}
public void M2(string s)
{
M(s);
s.ToString(); //warning CS8602: Dereference of a possibly null reference.
}
}sharplab #Resolved |
I think that's covered by |
It's oblivious because we have a bug in type substitution. Fixing that bug introduces a difficult cycle. Filed a follow-up issue. Details are in #41368 #Resolved |
|
Addressed the following comments:
Fixed/added. Thanks
Leaving as-is for now. We can tweak diagnostics as we go. #Resolved |
|
This PR flagged more issues in bootstrapping that were introduced in |
| { | ||
| RoslynDebug.Assert(other is object); |
There was a problem hiding this comment.
RoslynDebug.Assert(other is object) [](start = 12, length = 35)
This is a public method so we should handle null. #Resolved
There was a problem hiding this comment.
Thanks, indeed. I'll add a null check (since implementation below would crash on dereference...)
In reply to: 377720266 [](ancestors = 377720266)
| @@ -18,12 +19,13 @@ internal MemberRefComparer(MetadataWriter metadataWriter) | |||
| _metadataWriter = metadataWriter; | |||
| } | |||
|
|
|||
| public bool Equals(ITypeMemberReference x, ITypeMemberReference y) | |||
| public bool Equals([AllowNull] ITypeMemberReference x, [AllowNull] ITypeMemberReference y) | |||
There was a problem hiding this comment.
[AllowNull] ITypeMemberReference [](start = 27, length = 32)
Consider using ITypeMemberReference? rather than [AllowNull]. #Resolved
| @@ -18,12 +19,13 @@ internal MethodSpecComparer(MetadataWriter metadataWriter) | |||
| _metadataWriter = metadataWriter; | |||
| } | |||
|
|
|||
| public bool Equals(IGenericMethodInstanceReference x, IGenericMethodInstanceReference y) | |||
| public bool Equals([AllowNull] IGenericMethodInstanceReference x, [AllowNull] IGenericMethodInstanceReference y) | |||
There was a problem hiding this comment.
[AllowNull] IGenericMethodInstanceReference [](start = 27, length = 43)
IGenericMethodInstanceReference? #Resolved
| @@ -17,7 +18,7 @@ internal TypeSpecComparer(MetadataWriter metadataWriter) | |||
| _metadataWriter = metadataWriter; | |||
| } | |||
|
|
|||
| public bool Equals(ITypeReference x, ITypeReference y) | |||
| public bool Equals([AllowNull] ITypeReference x, [AllowNull] ITypeReference y) | |||
There was a problem hiding this comment.
[AllowNull] ITypeReference [](start = 27, length = 26)
ITypeReference? #Resolved
Compilers.sln
Outdated
| @@ -1,6 +1,6 @@ | |||
| Microsoft Visual Studio Solution File, Format Version 12.00 | |||
| # Visual Studio 15 | |||
| VisualStudioVersion = 15.0.27102.0 | |||
| # Visual Studio Version 16 | |||
There was a problem hiding this comment.
Thanks for catching that. My compilers.sln keeps getting updated recently and it's been error-prone to avoid adding it... :(
Implements the last part of #36039 (new enforcement of nullable attributes).
In terms of design:
[Maybe/NotNull]attributes on by-value/inparameters are matching